Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Test CheckForServiceAvailability #25

Merged
merged 9 commits into from
Mar 28, 2024
Merged

Test CheckForServiceAvailability #25

merged 9 commits into from
Mar 28, 2024
+306 −34

Conversation

kavir1698
Copy link
Collaborator

@kavir1698 kavir1698 commented Mar 27, 2024 •
edited
Loading

Will close #23

Tag the repo with in increased path version after merging this.

@kavir1698 kavir1698 marked this pull request as draft March 27, 2024 14:59
@kavir1698 kavir1698 marked this pull request as ready for review March 27, 2024 15:58
@kavir1698 kavir1698 requested a review from a team March 27, 2024 15:58
@kavir1698 kavir1698 changed the title Test check for service availability Test checkForServiceAvailability Mar 27, 2024
@kavir1698 kavir1698 changed the title Test checkForServiceAvailability Test CheckForServiceAvailability Mar 27, 2024
sbliven
sbliven reviewed Mar 27, 2024
View reviewed changes
Copy link
Member

@sbliven sbliven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downtime notifications are a nice feature. Did this already exist before? I never knew to update the availability status.

I'm not sure that storing the file in the main branch is the best way to go. This feels like mixing code with devops, although it's temptingly straightforward.

Thanks for the tests!

datasetUtils/checkForServiceAvailability.go Show resolved Hide resolved
@kavir1698
Copy link
Collaborator Author

Yes this was already there, I just cleaned the code and changed paths to Github.

Where would you store the yml file?

@sbliven
Copy link
Member

sbliven commented Mar 27, 2024

Maybe a separate branch?

Let's merge this as is and discuss next week.

@minottic
Copy link
Collaborator

I have the feeling as well this is added complexity for no real reason. A better way to do this, IMHO, would be to make the BE return the unaivalabiltity and log it here (or maybe just simply notify the users). TBH I would get rid of this logic, but indeed we can do in another PR.

Something else though. I think this requires the file ...available.txt which is missing from the release. So this will always fail right? should we merge the one with it first?

@kavir1698
Copy link
Collaborator Author

think this requires the file ...available.txt which is missing from the release.

Sorry I don't see what file you're talking about. It checks the service availability from datasetIngestorServiceAvailability.yml.

@minottic
Copy link
Collaborator

yes, that one. Ah ok I see it now on github. I got confused by this and was looking for it in the release

@kavir1698
Copy link
Collaborator Author

Ok then I'll merge this one and later we can talk about hosting the yml file on a different branch.

@kavir1698 kavir1698 merged commit 325dd1e into main Mar 28, 2024
2 checks passed
@kavir1698 kavir1698 deleted the test_CheckForServiceAvailability branch March 28, 2024 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbliven sbliven sbliven approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update CheckForServiceAvailability to read from Github
3 participants
@kavir1698 @sbliven @minottic